Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow multiple summaries per pull request #53

Merged
merged 17 commits into from
Jan 29, 2024

Conversation

lagru
Copy link
Member

@lagru lagru commented Nov 10, 2023

Tackles the first case in #45:

1pr-to-many: A pull requests addresses multiple things which should ideally appear as multiple summaries. Possibly even in separate sections.

It would be good to have this in before the next release of scikit-image. So that changelist can gracefully deal with scikit-image/scikit-image#6695.

@lagru lagru added the type: Enhancement New feature or request label Nov 10, 2023
Copy link
Member Author

@lagru lagru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, this is a first working draft and still rough around the edges / missing documentation. But see the comments for some early discussion & context.

README.md Outdated Show resolved Hide resolved
src/changelist/_format.py Outdated Show resolved Hide resolved
src/changelist/_format.py Outdated Show resolved Hide resolved
The new dataclass ChangeNote helps with uncoupling the `_query.py` logic
and the `_format.py` logic from each other somewhat. In this case
one pull request can be turned into multiple change notes which might
appear in separate sections. This optional behavior allows to deal with
somewhat large or unfocused pull requests.

Long-term this step would also be necessary in case PyGitHub is replaced
with a GraphQl-based approach.
The new dataclass Contributor helps with uncoupling the `_query.py` logic
and the `_format.py` logic from each other somewhat.

Long-term this step would also be necessary in case PyGitHub is replaced
with a GraphQl-based approach.
Querying and formatting might generate warnings. Make sure that these
are printed before the generated notes.
This is nothing to fancy but it should catch unexpected changes in
our formatting logic and cover the happy path.
@lagru lagru force-pushed the multiple-summaries branch 3 times, most recently from 9e88b20 to bc5fef4 Compare November 14, 2023 00:10
@lagru lagru marked this pull request as ready for review November 14, 2023 00:14
@lagru
Copy link
Member Author

lagru commented Nov 14, 2023

Okay, finally Python 3.9 is happy (sorry for the notification noise). I think this is ready to merge.

As a real world example, this

changelist scikit-image/scikit-image 54316d5~1 54316d5

includes just the merge commit from scikit-image/scikit-image#6695 and creates the following notes:

Details
# scikit-image x.y.z

We're happy to announce the release of scikit-image x.y.z!

## New Features

- Add parameter `mode` to `binary_erosion`, `binary_dilation`, `binary_opening` and `binary_closing` in`skimage.morphology`. These new parameters determine how array borders are handled ([#6695](https://github.com/scikit-image/scikit-image/pull/6695)).
- Add parameters `mode` and `cval` to `erosion`, `dilation`, `opening`, `closing`, `white_tophat`, and `black_tophat` in `skimage.morphology`. These new parameters determine how array borders are handled ([#6695](https://github.com/scikit-image/scikit-image/pull/6695)).
- Add functions `mirror_footprint` and `pad_footprint` to `skimage.morphology` ([#6695](https://github.com/scikit-image/scikit-image/pull/6695)).

## API Changes

- Parameters `shift_x` and `shift_y` in `skimage.morphology.erosion` and `skimage.morphology.dilation` are deprecated. Use `pad_footprint` or modify the footprint manually instead ([#6695](https://github.com/scikit-image/scikit-image/pull/6695)).

## Bug Fixes

- Ensure `skimage.morphology.closing` and `skimage.morphology.opening` are extensive and anti-extensive, respectively, if the footprint is not mirror symmetric ([#6695](https://github.com/scikit-image/scikit-image/pull/6695)).

## Contributors

2 authors added to this release (alphabetically):

- Cris Luengo ([@crisluengo](https://github.com/crisluengo))
- Lars Grüter ([@lagru](https://github.com/lagru))

3 reviewers added to this release (alphabetically):

- Cris Luengo ([@crisluengo](https://github.com/crisluengo))
- Lars Grüter ([@lagru](https://github.com/lagru))
- Stefan van der Walt ([@stefanv](https://github.com/stefanv))

_These lists are automatically generated, and may not be complete or may contain
duplicates._

Let me know what you think. :)

@lagru
Copy link
Member Author

lagru commented Nov 28, 2023

@stefanv, could I ask you to have a look at this if you have the time? 🙏 Not the highest priority in my opinion but I'd hate to rush this just before our next skimage release.

Copy link
Member

@stefanv stefanv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @lagru, this looks good overall. I suggested a syntax change to how the label is indicated.

.github/workflows/test.yml Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
src/changelist/_format.py Outdated Show resolved Hide resolved
src/changelist/_format.py Outdated Show resolved Hide resolved
lagru and others added 2 commits November 29, 2023 09:06
Co-authored-by: Stefan van der Walt <[email protected]>
Co-authored-by: Stefan van der Walt <[email protected]>
@stefanv
Copy link
Member

stefanv commented Nov 29, 2023

@lagru Let me know if you intend to make any other changes, otherwise I'll merge.

This syntax should hopefully be more robust than the previous one based
on the relatively common ":" character.
@lagru
Copy link
Member Author

lagru commented Jan 9, 2024

@stefanv. I've addressed all your comments and switched to the {label="Some label"} based syntax. This {...} block can be added anywhere once in the summary. So it can be used as a fenced_code_attributes that is hidden, or anywhere else so that it's visible.

I've also added basic tests for this in test_objects.py.

@lagru
Copy link
Member Author

lagru commented Jan 21, 2024

As we talked about, I've added a section in the README to document the new feature and make it clearer how to use it. It's duplicating the documentation in the config a little bit, but it's probably less hidden this way and should read nicer (no talk of regex).

@lagru lagru requested a review from stefanv January 21, 2024 10:32
Copy link
Member

@stefanv stefanv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Is overriding the title documented?

Comment on lines +43 to +46
By default, changelist will fall back to the title of a pull request and its
GitHub labels to sort it into the appropriate section. However, if you want
longer summaries of your changes you can add a code block with the following
form anywhere in the description of the pull request:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stefanv, re #53 (review): yes, overriding the title is documented.

README.md Outdated Show resolved Hide resolved
Co-authored-by: Stefan van der Walt <[email protected]>
@stefanv stefanv merged commit 3864506 into scientific-python:main Jan 29, 2024
5 checks passed
@stefanv
Copy link
Member

stefanv commented Jan 29, 2024

Thanks Lars!

@jarrodmillman jarrodmillman added this to the 0.5 milestone Jan 29, 2024
@lagru lagru deleted the multiple-summaries branch January 29, 2024 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants